Conversation
b201372 to
777b8ba
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces hash-based redaction as an opt-in feature that allows correlating log entries while preserving privacy. When enabled, sensitive values marked with the HashValue interface are hashed (using SHA-1/HMAC-SHA1) instead of being fully redacted, with the hash output denoted by a dagger (†) prefix in redaction markers.
Key Changes:
- Adds
EnableHashing(salt)API to enable hash-based redaction with optional HMAC salt - Introduces
HashValueinterface and wrapper types (HashString,HashInt, etc.) for marking values as hashable - Implements hash computation logic using SHA-1/HMAC-SHA1 with truncation to 8 hex characters
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| api.go | Exports new HashValue types and EnableHashing function to public API |
| interfaces/interfaces.go | Defines HashValue marker interface and concrete wrapper types for hashable values |
| internal/markers/constants.go | Adds HashPrefix constant (†) and updates regex patterns to handle hash markers |
| internal/markers/hash.go | Implements hash computation logic with SHA-1/HMAC-SHA1 and global state management |
| internal/markers/hash_test.go | Adds unit tests for low-level hash functions with various inputs and salt configurations |
| internal/markers/markers.go | Updates Redact() methods to detect and process hash markers (‹†value›) |
| internal/rfmt/print.go | Adds HashValue interface detection in three locations to wrap values with hash markers during formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
53b3995 to
365e09b
Compare
dhartunian
left a comment
There was a problem hiding this comment.
This is great! I will review more thoroughly next week. Thanks for working on this.
This commit introduces the ability to hash sensitive information instead
of fully redacting it. This let's the user correlate entries while still
preserving privacy. This is an opt-in feature and requires the user to explicitly mark
values as "hashable".
Changes:
* EnableHashing(salt) is used to turn on hash-based redaction. If this is
not called, redaction behaves as before (full redaction).
* If both SafeValue and HashValue interfaces are implemented on a type,
SafeValue takes precedence.
* Uses SHA-1 / HMAC-SHA1 (if salt is provided) to hash the sensitive
parts of the string.
* A dagger (†) prefix is added within the redaction markers to denote
that it should be hashed instead of fully redacted.
* There are 2 ways to mark a value "hashable" (similar to redaction):
1) Implementing the HashValue interface directly on a type.
2) Wrapping a value with the HashString/HashBytes/Hash* functions
* Hashes are calculated at formatting time and not during string
creation. Just like redaction.
**Example usage**
* Implementing the HashValue interface:
```go
type UserID string
func (u UserID) HashValue() string {} // makes the type "hashable"
fmt.Println(redact.Sprintf("%s", UserID("alice")).Redact()) // Output: ‹522b276a›
```
* Inline with helper functions:
```go
fmt.Println(redact.Sprintf("%s", redact.HashString("alice")).Redact()) // Output: ‹522b276a›
```
365e09b to
74df909
Compare
|
Ack. Will address the comments end of the week. |
dhartunian
left a comment
There was a problem hiding this comment.
This PR also contains no tests for any redactable and hashed strings and mixed hashed redactable stuff, etc. Huge test coverage gap.
aa-joshi
left a comment
There was a problem hiding this comment.
@aa-joshi reviewed 4 files and made 3 comments.
Reviewable status: 4 of 9 files reviewed, 17 unresolved discussions (waiting on Abhinav1299, arjunmahishi, and dhartunian).
internal/markers/hash.go line 1 at r4 (raw file):
// Copyright 2025 The Cockroach Authors.
nit: 2026
internal/markers/markers.go line 94 at r4 (raw file):
// they are redacted like regular markers. func (s RedactableBytes) Redact() RedactableBytes { result := ReStripSensitive.ReplaceAllFunc([]byte(s), func(match []byte) []byte {
same as here: https://reviewable.io/reviews/cockroachdb/redact/33#gh-2823788809
eae80f4 to
12de922
Compare
12de922 to
576761f
Compare
dhartunian
left a comment
There was a problem hiding this comment.
Nice work. Just a few more edge cases and then we should be good go. Thanks!
… concurrency test
9f5154f to
95c49a1
Compare
dhartunian
left a comment
There was a problem hiding this comment.
LGTM, just one nit. Thanks!
| "‹×›", | ||
| }, | ||
| { | ||
| "hash with space separator", |
There was a problem hiding this comment.
nit: this needs a space, right?
This commit introduces the ability to hash sensitive information instead of fully redacting it. This let's the user correlate entries while still preserving privacy. This is an opt-in feature and requires the user to explicitly mark values as "hashable".
Changes:
EnableHashing(salt)is used to turn on hash-based redaction. If this isnot called, redaction behaves as before (full redaction).
SafeValueandHashValueinterfaces are implemented on a type,SafeValuetakes precedence.SHA-256/HMAC-SHA256(if salt is provided) to hash the sensitiveparts of the string.
†) prefix is added within the redaction markers to denotethat it should be hashed instead of fully redacted.
HashValueinterface directly on a type.HashString/HashBytes/Hash*functionscreation. Just like redaction.
Example usage
Implementing the
HashValueinterface:Inline with helper functions:
Benchmark of hashing enabled vs disabled
This change is